Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tools: add preinstall script for npm on OS X #3986

Closed
wants to merge 1 commit into from
Closed

tools: add preinstall script for npm on OS X #3986

wants to merge 1 commit into from

Conversation

evanlucas
Copy link
Contributor

Make sure we cleanly remove npm before installing on OS X

Related: #3606

This is working for me locally, but would be great to have some others test.

@Fishrock123
Copy link
Contributor

Notes: I couldn't reproduce the original anymore, so I don't know how we can confirm this will actually work?

@Fishrock123 Fishrock123 added the install Issues and PRs related to the installers. label Nov 23, 2015
@evanlucas
Copy link
Contributor Author

Good point. I'll see if I can spin up a vm and try to reproduce

@mscdex mscdex added the macos Issues and PRs related to the macOS platform / OSX. label Nov 23, 2015
@Fishrock123 Fishrock123 mentioned this pull request Dec 4, 2015
Closed
rvagg
rvagg reviewed Dec 8, 2015
View reviewed changes
tools/osx-pkg-preinstall.sh
# TODO Can this be done inside the .pmdoc?
# TODO Can we extract $PREFIX from the installer?
cd /usr/local/bin
rm -rf ../lib/node_modules/npm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we doing a cd followed by rm and not rm with absolute path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow, good question. That is how the postinstall script is done. And now that I look again, it was done that way for a reason. Updating now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we also need a set -e if we're doing multiple lines, if the cd fails then the rm will still execute

@evanlucas
tools: add preinstall script for npm on OS X
8ac2af7 
Make sure we cleanly remove npm before installing on OS X
@evanlucas
Copy link
Contributor Author

Ok, updated with absolute path and set -e

@rvagg
Copy link
Member

rvagg commented Dec 8, 2015

test build of this created @ https://nodejs.org/download/test/v6.0.0-test201512088ac2af7628/ if anyone else wants to review the changes

@rvagg
Copy link
Member

rvagg commented Dec 8, 2015

I touched a bunch of TEST files under /usr/local/lib before running the installer and they still exist:

$ node -v
v6.0.0-test201512088ac2af7628
$ find /usr/local -name TEST
/usr/local/lib/node_modules/npm/node_modules/TEST
/usr/local/lib/node_modules/npm/TEST
/usr/local/lib/node_modules/TEST
/usr/local/lib/TEST

So something's not right with this as far as I can tell.

@rvagg rvagg mentioned this pull request Dec 8, 2015
Closed
@rvagg
Copy link
Member

rvagg commented Dec 8, 2015

Perhaps it needs to be "preupgrade" as well as "preinstall". Reading here http://s.sudre.free.fr/Stuff/PackageMaker_Howto.html but I don't know how authoritative this is.

@evanlucas
Copy link
Contributor Author

@rvagg Ok yes it looks like neither the preinstall, nor the postinstall scripts were running. What is weird to me is that the local.pkg installs npm, so there really is no need to have an npm.pkg. Even if one deselects the npm choice, npm will still be installed. I can make it a single package and then the scripts work properly. I guess my question becomes, is it worth it? With how outdated PackageMaker.app is, I really think that we should try to get #2571 landed. Is there anything blocking that?

@Fishrock123
Copy link
Contributor

@evanlucas I think the other issue mostly needs review.

@Fishrock123 Fishrock123 mentioned this pull request Jan 12, 2016
Closed
@evanlucas
Copy link
Contributor Author

Closing. Hopefully we can get the new OS X pkg stuff in soon. It should replace this anyways

@evanlucas evanlucas closed this Feb 24, 2016
@evanlucas evanlucas deleted the pkgs branch February 24, 2016 12:48
rvagg pushed a commit to rvagg/io.js that referenced this pull request Jun 8, 2016
@fhemberger @rvagg
Add npm preinstall script
d876918 
See nodejs#3986
This was referenced Aug 11, 2020
Merged
Open
Merged
Merged
@snyk-bot snyk-bot mentioned this pull request Oct 22, 2020
Open
@hixio-mh hixio-mh mentioned this pull request Jan 22, 2021
Merged
This was referenced Mar 16, 2021
Merged
Open
This was referenced Mar 22, 2022
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
This was referenced Aug 19, 2022
Open
Open
Open
@bsmoove247 bsmoove247 mentioned this pull request Oct 6, 2022
Open
@Verdinjoshua26 Verdinjoshua26 mentioned this pull request Oct 8, 2022
Open
@aliscco aliscco mentioned this pull request Jun 21, 2023
Open
This was referenced Nov 29, 2023
Open
Open
Open
This was referenced May 14, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
install Issues and PRs related to the installers. macos Issues and PRs related to the macOS platform / OSX.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@evanlucas @Fishrock123 @rvagg @mscdex